-
Notifications
You must be signed in to change notification settings - Fork 288
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow reclaiming the current allocation #686
Conversation
This is a draft to get some comments on whether such an API addition would be acceptable |
This looks a lot like |
I think it is hard to know what the original capacity is, generally, as it could've been influenced by lots of hidden operations before. |
That's true, but presumably you know how much data you'll need to reserve regardless (otherwise dynamic allocation seems like the best strategy). So it doesn't particularly matter if you can reclaim the entire buffer, only if it can fit the data you're receiving. Or am I missing the point here?
Very cheap relative to what though? Based on your description in #680, your only other alternative would be to allocate anyway, right? And that's definitely expensive relative to moving bytes. If that's not the alternative, what is? |
My usecase is that I have a couple of |
rebased this on top of current master, to make it mergable again. But I am also not sure if it is deemed merge-worthy or not :) There is currently a test case failure because BytesMut::advance() changed from its documented behaviour, but if this is generally acceptable I would try to reconcile that. |
In my mind, reclaiming memory is always in service of then writing a specific amount of data so I would prefer a |
Wouldn't that just be |
|
Ah, I misunderstood. Yeah, that'd work as well for me! I will update the branch when I get a chance |
Naming nit: The name |
This fixes tokio-rs#680, where it was noted that it is hard to use BytesMut without additional allocations in some circumstances.
Implemented the suggestion, I kept the name |
This has the nice side effect of making the implementation simpler with regards to unsafe code, as it mostly follows what `reserve` does.
Thanks for the reviews :) Added a commit implementing copying bytes if that is cheap according to |
We should fix this expect when allocate is false: |
Under the current strategy of inlining, there's only one large instance of reclaim_inner in the resulting binary. Does it maybe make sense to add an intermediate inline target for try_reclaim_inner that can optimise to a much smaller function? Eg: #[inline]
pub fn try_reclaim(&mut self, additional: usize) -> bool {
let len = self.len();
let rem = self.capacity() - len;
if additional <= rem {
return true;
}
self.try_reclaim_inner(additional)
}
fn try_reclaim_inner(&mut self, additional: usize) -> bool {
// inlines
self.reserve_inner(additional, false)
} |
Implementing the suggestion by @conradludgate
Thank you! Implemented a fix to no longer panic when trying to reclaim large amounts of storage. I am not sure about the inlining, tbh - we could also inline the helper function at the expense of making try_reclaim a bit larger. Did you play with it already to see what would make most sense? |
I only briefly scanned some generated assembly. No benchmarks or profiling. I think keeping what is currently here is fine and won't make a considerable different to performance. If you only use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. Good catch with the panic.
This is based on #680, where it was noted that it is hard to use BytesMut without additional allocations in some circumstances.